Skip to content

perf: check for subscribers before building the per-call context in wrappers - #102

Open
pabloerhard wants to merge 1 commit into
nodejs:mainfrom
pabloerhard:pabloerhard/orch-fast-path
Open

pabloerhard wants to merge 1 commit into
nodejs:mainfrom
pabloerhard:pabloerhard/orch-fast-path

Conversation

@pabloerhard

@pabloerhard pabloerhard commented Oct 1, 2026 •

Copy link
Copy Markdown

What

Generated wrappers built their per-call transport on every call, even with no subscribers: the arguments array plus its .slice(0, arguments.length) copy, the __apm$ctx object, and the __apm$traced closure. They only checked tr_ch_apm_hasSubscribers after that. Callback wrappers also ran Array.prototype.at and created the hoisted __apm$wrappedCb closure first, Auto ran at first, and patched iterator methods copied arguments before checking.

This moves the check to just after the __apm$traced declaration, which now receives the argument list as a parameter. An unsubscribed call goes straight to the original with arguments (or [params] when the wrapper is an arrow, which has no own arguments). The now-duplicate checks are removed from the Sync/Async templates, __apm$wrappedCb becomes a const function expression, and iterator methods check before copying.

The AST shape that idempotency and downstream custom transforms depend on is unchanged: const __apm$traced = <arrow> holding const __apm$wrapped = <fn>, a top-level __apm$ctx object, a top-level if (!tr_ch_apm_hasSubscribers(ch)) return __apm$traced(...) check, and two __apm$traced calls in Sync/Async wrappers.

Why behaviour is unchanged

  • No subscribers: nothing can observe or change the arguments, so skipping the transport has no visible effect.
  • Subscribed: the callee gets the same __apm$arguments array after start, so in-place mutation (replace, append, callback splice) still reaches it.
  • Arguments: the wrapper has a rest parameter, so its arguments lists exactly what [...].slice(0, arguments.length) produced.
  • Constructors: before, the fast path got an unsliced array padded with undefined. Parameters, defaults and rest bind the same either way, and arguments inside the moved arrow body is still the constructor's own. new.target is unchanged.
  • this is still lexical through the arrow __apm$traced. Generator and async flags stay on the inner function.
  • __apm$super setup is still added above everything.
  • Check-then-publish: the same check is read once, and nothing user-visible runs between it and runStores.
  • Callback/Auto: keep their start-only check after the full one, so they take the fast path only when the old code would have.
  • Subscribed calls: the contract is unchanged: a fresh ctx per call, start via runStores, end in finally, ctx.error set before error publishes, and result replacement.

Tests

  • fast_path_cjs: every kind (Sync, Async, Callback, Auto, Iterator/AsyncIterator returnKind, class method, base and derived constructor, arrow expression, runtime-patched instance method) with fewer, exact and more arguments. It goes unsubscribed → subscribed → unsubscribed and spies on slice/at to assert no allocation on the fast path. It also pins iterator next with only the main channel subscribed, and Callback/Auto with only asyncEnd subscribed.
  • arguments_mutation_kinds_cjs: subscribed in-place mutation for Sync, Async, Callback and constructors.
  • fast path ordering: checks the generated code for the ordering above.

The behaviour assertions pass unchanged on main; only the allocation and ordering checks fail there.

Benchmarks

Node v25.0.0, Apple M4 Max, 11 fresh processes per side, median ns/op:

unsubscribed before after Δ minor GCs / 1M calls
Sync 31.60 11.73 −62.9% 16.4 → 7.6
Async 31.03 11.76 −62.1% 18.6 → 10.2
Callback 48.66 12.10 −75.1% 17.0 → 9.0
Auto 48.78 12.42 −74.5% 17.2 → 9.0
class method 29.23 11.75 −59.8% 16.0 → 7.6
derived constructor 32.63 21.77 −33.3% 14.6 → 11.6
Iterator (call + next()) 427.84 391.14 −8.6% 3.0 → 2.4

Subscribed calls are within noise. With all five events and noop handlers: Sync −1.8%, Async +1.9%, Callback −0.3%, Auto −0.3%, method −1.8%, ctor +1.7%, Iterator −1.1% (spread per side is 2–13%). With an AsyncLocalStorage bound to start: Sync +0.4%, Async −0.0%, Callback −0.3%, Auto −0.2%, method −0.1%, ctor +0.0%.

Follow-ups (not in this PR)

  • Hoist the original out of the wrapper for plain function declarations. Measured at about 12 → 7.6 ns more.
  • Decide whether Callback/Auto should use the full five-event check instead of start-only. That would change behaviour.

Signed-off-by: Pablo Erhard <pablo.erhardhernandez@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant